-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add list_parameters & test #1124
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementation looks good to me, probably we can add more test cases.
@fujitatomoya Hey, I want to contribute for the first time, is there still a need for more testcases for this issue ? |
@leeminju531 sorry to be late to get back to this. #1124 (comment) makes sense to me, could you address that? after that we can start the CI again. |
It appears to be a reasonable suggestion, and I'm fully prepared to make the necessary updates in the near future. |
I have made revisions according to the suggestions. I kindly request @clalancette and @fujitatomoya to review the changes. |
@clalancette your comment is addressed, can you check again? |
The more I look at this one, the more I'm curious why the implementation can't look like the rclcpp one. If it was more-or-less a Python port of that, it would be much easier to read (and reason about), since the logic would be the same. @leeminju531 Can you maybe describe why the current implementation is like it is, rather than a direct port of the rclcpp code? |
I referred to _list_parameters_callback, where a similar process I intend to adopt has already been implemented ;however, i understood and agree your comments. I've observed that the service callbacks of ParameterService mostly directly call the functions owned by the node, except for _list_parameters_callback, which is implemented directly. i believe it should also referred to the function the node owns for consistency and maintainability. I believe that the modification should be made in conjunction with your proposed comment. |
Yes, I think that all makes a lot of sense to me. To recap:
Please go ahead and do that, and I'm happy to review the result! |
Signed-off-by: leeminju531 <[email protected]>
Please review the changes at your convenience. @clalancette |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks great. I've left one comment inline; I'm going to hold off on merging it until we figure out who is going to submit the PR to rclcpp. But I'm going to run CI on this and get it prepared for merge.
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
This addresses the issue (#1097) by implementing the functionality for list_parameters that the node owns.
This feature should help with logging and debugging by allowing users to easily retrieve all declared parameters and their values, similar to the functionality in ROS1.